Skip to content

Fix blocking calls for gRPC even though using Reactor#435

Merged
artursouza merged 5 commits into
dapr:masterfrom
xiazuojie:master
Jan 12, 2021
Merged

Fix blocking calls for gRPC even though using Reactor#435
artursouza merged 5 commits into
dapr:masterfrom
xiazuojie:master

Conversation

@xiazuojie

@xiazuojie xiazuojie commented Jan 7, 2021 •
edited by artursouza
Loading

Copy link
Copy Markdown
Contributor

Description

Make DaprClientGrpc truly async and update unit tests.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Reference: #434

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation

@ghost

ghost commented Jan 7, 2021 •
edited by ghost
Loading

Copy link
Copy Markdown

CLA assistant check
All CLA requirements met.

artursouza
artursouza suggested changes Jan 8, 2021
View reviewed changes

@artursouza artursouza left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR, it is promising. There are two points of contention for me here. First, the unit tests for hot mono should stay - that is a big deal when dealing with Reactor and we found it the hard way early in this project. Second, we should not wrap (or catch) throwables otherwise we end up in situations where we handle or wrap OutOfMemoryError, for example.

Comment thread sdk/src/main/java/io/dapr/exceptions/DaprException.java
Comment thread sdk/src/test/java/io/dapr/client/DaprClientGrpcTest.java
@artursouza

artursouza commented Jan 11, 2021

Copy link
Copy Markdown
Contributor

The only thing now is to bring back the hot mono unit tests.

@codecov

codecov Bot commented Jan 12, 2021

Copy link
Copy Markdown

Codecov Report

Merging #435 (12ed4e5) into master (9e13586) will decrease coverage by 0.13%.
The diff coverage is 85.54%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #435      +/-   ##
============================================
- Coverage     83.44%   83.30%   -0.14%     
- Complexity      851      859       +8     
============================================
  Files            74       74              
  Lines          2519     2540      +21     
  Branches        256      256              
============================================
+ Hits           2102     2116      +14     
- Misses          296      302       +6     
- Partials        121      122       +1
Impacted Files Coverage Δ Complexity Δ
...k/src/main/java/io/dapr/client/DaprClientGrpc.java 83.28% <85.00%> (-0.27%) 94.00 <22.00> (+6.00) ⬇️
...rc/main/java/io/dapr/exceptions/DaprException.java 73.46% <85.71%> (-4.58%) 17.00 <6.00> (+2.00) ⬇️
...rc/main/java/io/dapr/client/DaprClientBuilder.java 70.96% <100.00%> (ø) 8.00 <0.00> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9e13586...68efcca. Read the comment docs.

@artursouza artursouza merged commit 76d877c into dapr:master Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@artursouza artursouza artursouza approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@xiazuojie @artursouza